fix(server): return 404 if get backing image and backup not found #2067

ChanYiLin · 2023-07-10T09:13:38Z

ref: longhorn/longhorn#6266

shuo-wu

Do we need to handle volume/backupvolume for CSI API DeleteSnapshot?
The idea LGTM.

api/backingimage.go

api/backup.go

ChanYiLin · 2023-07-11T04:04:14Z

VolumeSnapshot has 3 types

backing image
snapshot
backup

When deleting

csi: try to get the CR first, it it doesn't exist, then pass
client: check if 404, return CR=nil
server: return 404, if it cannot get the CR from datastore

innobead

LGTM.

innobead · 2023-07-11T05:39:34Z

@shuo-wu Please help review this.

ChanYiLin · 2023-07-11T05:42:58Z

@innobead
I am running the e2e test, and it failed at backup snapshot
Let me check for it

innobead · 2023-07-11T05:43:57Z

NP. Passing the test is the criteria before merging.

ChanYiLin · 2023-07-11T08:50:01Z

weird, it passed the test in my environment
need some time to investigate

ref: longhorn/longhorn 6266 Signed-off-by: Jack Lin <jack.lin@suse.com>

ChanYiLin · 2023-07-11T11:03:15Z

I know the reason why
Because I used /v1/backups/XXX in our generated client (link) to try to get the backup before deleting it
Turns out, we don't provide such API in our server
So csi thought there was no such backup and pass the deletion, and failed when checking if the backup still existed in the python test.

ChanYiLin · 2023-07-11T11:05:10Z

We don't have to check by getting the backup or snapshot before deletion
because in the delete api we already pass the deletion if it is not found

longhorn-manager/api/backup.go

Lines 125 to 134 in 75d2e77

    
           backup, err := s.m.GetBackup(input.Name, volName) 
        
           if err != nil { 
        
           	logrus.WithError(err).Warnf("failed to get backup '%v' of volume '%v'", input.Name, volName) 
        
           } 
        
           if backup != nil { 
        
           	if err := s.m.DeleteBackup(input.Name, volName); err != nil { 
        
           		return errors.Wrapf(err, "failed to delete backup '%v' of volume '%v'", input.Name, volName) 
        
           	} 
        
           }

so removed the check

ChanYiLin · 2023-07-11T11:07:31Z

e2e running
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4550/console

…g_and_backup_resource

shuo-wu · 2023-07-11T13:13:24Z

We don't have to check by getting the backup or snapshot before deletion
because in the delete api we already pass the deletion if it is not found
longhorn-manager/api/backup.go
so removed the check

Sorry... Does the PR contain the removal (of getting backing image/backup volume before requesting deletion)...? BTW, for the snapshot deletion, getting volume is unavoidable. We still need to make sure getting a nil volume should be handled as expected.

ChanYiLin · 2023-07-11T13:46:03Z

Hi @shuo-wu
No, this PR maintains the same as before, I didn't remove anything in csi part
And only add 404 not found header in server side
I am saying, for csi part, I have removed all the changes I made earlier since the deletion api for backup and snapshot already check if it exists

ChanYiLin · 2023-07-11T13:48:25Z

e2e tests passed
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4550/

shuo-wu · 2023-07-11T13:50:56Z

OK. I was thinking you directly remove the backup volume/backing image get calls from the CSI SnapshotDelete implementation. But this is an alternative solution for this issue while making the implementation simpler. WDTY?

ChanYiLin · 2023-07-11T13:56:21Z

I think if from server side it already checks if it exists before deleting it
We don't need to do it in client side again
and if we do it in client side, we have to make sure every client call has checked if it exists?
I would say it adds the burden to the client side.

ChanYiLin · 2023-07-11T14:07:15Z

In conclusion
For this issue, it only effect BackingImage, because it gets the BackingImage and then delete it
But the get api return 500 if it is not found instead of 404, the deletion process failed
In this PR, I add the 404 if get function has not found error then the clenup for BackingImage could pass.

For backup and snapshot, they have parent resources which are backupVolume and volume
CSI gets the parent resource first and then delete the backup and snapshot
The process would be

if volume or backupVolume doesn't exists -> get 404 not found error then bypass the deletion (already did before)
call api delete snapshot -> delete api already check if snapshot or backup exists, no need to check in client side again

innobead · 2023-07-11T17:42:00Z

So do we need to do any other code change here?

ChanYiLin · 2023-07-12T00:27:26Z

So do we need to do any other code change here?

No, I have tested it manually and through e2e
Three types of volumesnapshot can be successfully deleted in any case

shuo-wu · 2023-07-12T03:22:36Z

I mean, for backing image cleanup, since there is no parent resource, there is no need to invoke cs.apiClient.BackingImage.ById(backingImageParameters[longhorn.BackingImageParameterName]) before deletion. We can directly call cs.apiClient.BackingImage.Delete(backingImage)

ChanYiLin · 2023-07-12T03:47:50Z

@shuo-wu
But in csi we don't have longhorn CR clientset only have longhorn api client
so we manipulate the resources through api client
and for BackingImage delete api, it needs BackingImage object

Even we delete the BackingImage directly, if it doesn't exist, we still need to handle not found error
I don't think the process will be different

get -> if found -> delete
delete -> if not found error then pass

And besides this issue, we still need to add 404 not found in the server api for other cases, maybe there are some places in our code also use 404 to determine if api has not found error.

innobead · 2023-07-12T05:00:51Z

@mergify backport v1.5.x

mergify · 2023-07-12T05:00:54Z

backport v1.5.x

✅ Backports have been created

#2076 fix(server): return 404 if get backing image and backup not found (backport #2067) has been created for branch v1.5.x

ChanYiLin requested a review from a team as a code owner July 10, 2023 09:13

ChanYiLin mentioned this pull request Jul 10, 2023

[BUG] Can not delete type=bi VolumeSnapshot if related backing image not exist longhorn/longhorn#6266

Closed

shuo-wu previously approved these changes Jul 10, 2023

View reviewed changes

api/backingimage.go Outdated Show resolved Hide resolved

api/backup.go Outdated Show resolved Hide resolved

ChanYiLin dismissed shuo-wu’s stale review via 872307f July 11, 2023 04:01

ChanYiLin force-pushed the LH6266_server_should_return_404_for_backing_and_backup_resource branch from d80e226 to 872307f Compare July 11, 2023 04:01

ChanYiLin requested a review from innobead July 11, 2023 04:06

ChanYiLin force-pushed the LH6266_server_should_return_404_for_backing_and_backup_resource branch from 872307f to 9f28b19 Compare July 11, 2023 05:34

ChanYiLin changed the title ~~fix(server): return 404 if get backing image and backup not found~~ [WIP: e2e test]fix(server): return 404 if get backing image and backup not found Jul 11, 2023

innobead previously approved these changes Jul 11, 2023

View reviewed changes

innobead requested a review from shuo-wu July 11, 2023 05:39

fix(server): return 404 if get backing image and backup not found

a333f30

ref: longhorn/longhorn 6266 Signed-off-by: Jack Lin <jack.lin@suse.com>

ChanYiLin dismissed innobead’s stale review via a333f30 July 11, 2023 10:57

ChanYiLin force-pushed the LH6266_server_should_return_404_for_backing_and_backup_resource branch from f159724 to a333f30 Compare July 11, 2023 10:57

Merge branch 'master' into LH6266_server_should_return_404_for_backin…

2305f29

…g_and_backup_resource

ChanYiLin changed the title ~~[WIP: e2e test]fix(server): return 404 if get backing image and backup not found~~ fix(server): return 404 if get backing image and backup not found Jul 11, 2023

shuo-wu approved these changes Jul 12, 2023

View reviewed changes

innobead merged commit a523288 into longhorn:master Jul 12, 2023
3 checks passed

mergify bot mentioned this pull request Jul 12, 2023

fix(server): return 404 if get backing image and backup not found (backport #2067) #2076

Merged

ChanYiLin mentioned this pull request Jul 12, 2023

[BACKPORT][v1.5.1][BUG] Can not delete type=bi VolumeSnapshot if related backing image not exist longhorn/longhorn#6270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): return 404 if get backing image and backup not found #2067

fix(server): return 404 if get backing image and backup not found #2067

ChanYiLin commented Jul 10, 2023

shuo-wu left a comment

ChanYiLin commented Jul 11, 2023 •

edited

Loading

innobead left a comment

innobead commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023

innobead commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023

shuo-wu commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023

shuo-wu commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

innobead commented Jul 11, 2023

ChanYiLin commented Jul 12, 2023

shuo-wu commented Jul 12, 2023

ChanYiLin commented Jul 12, 2023

innobead commented Jul 12, 2023

mergify bot commented Jul 12, 2023 •

edited

Loading

fix(server): return 404 if get backing image and backup not found #2067

fix(server): return 404 if get backing image and backup not found #2067

Conversation

ChanYiLin commented Jul 10, 2023

shuo-wu left a comment

Choose a reason for hiding this comment

ChanYiLin commented Jul 11, 2023 • edited Loading

innobead left a comment

Choose a reason for hiding this comment

innobead commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023

innobead commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023 • edited Loading

ChanYiLin commented Jul 11, 2023 • edited Loading

ChanYiLin commented Jul 11, 2023

shuo-wu commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023 • edited Loading

ChanYiLin commented Jul 11, 2023

shuo-wu commented Jul 11, 2023

ChanYiLin commented Jul 11, 2023 • edited Loading

ChanYiLin commented Jul 11, 2023 • edited Loading

innobead commented Jul 11, 2023

ChanYiLin commented Jul 12, 2023

shuo-wu commented Jul 12, 2023

ChanYiLin commented Jul 12, 2023

innobead commented Jul 12, 2023

mergify bot commented Jul 12, 2023 • edited Loading

✅ Backports have been created

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

ChanYiLin commented Jul 11, 2023 •

edited

Loading

mergify bot commented Jul 12, 2023 •

edited

Loading